Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix reflame deploys, add example visual tests #457

Closed

Conversation

lewisl9029
Copy link
Contributor

@lewisl9029 lewisl9029 commented Mar 7, 2024

This PR fixes the broken Reflame deploys on dev. It was caused by a few things:

  • One of our transitive deps is using a git-based dependency which Reflame doesn't support yet. Fixed this through an override. More context here.
  • There were some references to images in the public folder, which Reflame also doesn't support yet. Fixed this by moving those images into src and using url imports. More context here.

Also took this opportunity to set up a few visual tests for the Avatar component to show y'all how testing works in Reflame works. Happy to remove it if you don't feel it's adding any value, but I'd love to get y'all to try it out for a bit and hear your thoughts if it's something you're interested in!

In addition to these basic screenshot tests, Reflame can also do interaction tests, let me know if you're interested in giving that a try to, happy to provide some examples to get y'all started. Cheers!

lewisl9029 and others added 8 commits March 6, 2024 16:40
Reflame doesn't currently support a public folder, partly because assets in public folders can't take advantage of content-hashes for semi-permanent caching.
Copy link

vercel bot commented Mar 7, 2024

@lewisl9029 is attempting to deploy a commit to the XMTP Labs Team on Vercel.

A member of the Team first needs to authorize it.

// This package is specified as a git dependency: https://github.com/web3-storage/w3up/blob/d6978d7ab299be76987c6533d18e6857f6998fe6/packages/access-client/package.json#L113
// which Reflame doesn't support yet
// Luckily looks like the only reason for doing this was to point to a fork that fixes types: https://github.com/fission-codes/one-webcrypto/commit/5148cd14d5489a8ac4cd38223870e02db15a2382
// which is irrelevant to the Reflame deploy, so we can safely just point to the original package
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some context here so we can remove this when they eventually upstream their types change.

@@ -12,7 +12,7 @@
width: 50px;
height: 50px;
transform: scale(0.1);
background-image: url("../../../../public/xmtp-icon.png");
background-image: url("../../../images/xmtp-icon.png");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to move this out of public folders since Reflame doesn't support it yet.

Part of the reason we don't support it yet is because assets in the public folder can't take advantage of content-hashing (e.g. turning xmtp-icon.png into xmtp-icon-somehash1234.png) to allow for semi-permanent caching without worrying about invalidation.

It didn't seem like any of the images currently in public needed to be there as far as I could tell, so I moved them out and updated all the references here. Let me know if you have concerns with this, happy to adjust.


export const Avatar_test = <Avatar url={xmtpIconUrl} />
export const AvatarWithAddress_test = <Avatar address="1234" />
export const AvatarLoading_test = <Avatar isLoading />
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all it takes to add visual tests in Reflame! :)

You can see the results in the GitHub checks page: https://github.com/xmtp-labs/xmtp-inbox-web/pull/457/checks?check_run_id=22371292908

Screenshot 2024-03-06 at 5 57 28 PM

Happy to remove it if you don't plan on making use of this at all. But would love to have y'all try it out and hear what you think if you're up for it!

@lewisl9029 lewisl9029 marked this pull request as ready for review March 7, 2024 02:06
@lewisl9029 lewisl9029 requested a review from a team March 7, 2024 02:06
@lewisl9029
Copy link
Contributor Author

I believe e2e tests might be failing due to missing env vars since this PR is coming from a fork? Feel free to clone the branch over and open a separate PR internally if that makes things easier.

@reflame reflame bot had a problem deploying to Preview April 24, 2024 20:04 Error
Copy link

vercel bot commented Apr 24, 2024

Deployment failed with the following error:

The provided GitHub repository does not contain the requested branch or commit reference. Please ensure the repository is not empty.

@rygine
Copy link
Contributor

rygine commented Jul 11, 2024

closing this PR for now as this repo is currently in maintenance mode.

@rygine rygine closed this Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants